Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow the skip api method for sliders and udpatemenus #1699

Merged
merged 2 commits into from
May 18, 2017

Conversation

rreusser
Copy link
Contributor

@rreusser rreusser commented May 18, 2017

This PR implements half of #1695: the skip method by allowing it as a method and otherwise living with it but mostly ignoring it.

/cc @etpinard @alexcjohnson @n-riesco

@alexcjohnson
Copy link
Collaborator

Can you add a test that the plotly_sliderchanged and plotly_buttonclicked events still fire with skip?

@rreusser
Copy link
Contributor Author

rreusser commented May 18, 2017

I've added a test to updatemenus but not to sliders. I can add to sliders, but it's a bit tricky (requires calculating slider click positions and working through DOM nodes) and I don't think it's a particularly high-value test since I've tested the defaults to make sure attributes are not filtered inappropriately and since the line that actually implements this is in the command code, not the component code.

@rreusser
Copy link
Contributor Author

To say that more clearly, as long as the string is coerced correctly, the component code is completely agnostic about what the value is, except to the extent that it feeds back into the state via the command observer, which doesn't apply to this case anyway since the command observer is tested to return that it doesn't know anything about the 'skip' command.

@alexcjohnson
Copy link
Collaborator

I've added a test to updatemenus but not to sliders

Works for me. All looks great! 💃

@rreusser rreusser merged commit f27885d into master May 18, 2017
@rreusser rreusser deleted the allow-skip-api-method branch May 18, 2017 12:50
@@ -20,7 +20,7 @@ var stepsAttrs = {

method: {
valType: 'enumerated',
values: ['restyle', 'relayout', 'animate', 'update'],
values: ['restyle', 'relayout', 'animate', 'update', 'skip'],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A little note on 'skip' in the description would've been nice. We can get to that later though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature something new
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants